NO TICKET: make sample_pt_id PK for chemistry_sampleinfo#365
NO TICKET: make sample_pt_id PK for chemistry_sampleinfo#365jacob-a-brown wants to merge 2 commits into
Conversation
object_id was mistakenly set as the PK
jacob-a-brown
requested review from
chasetmartin,
Copilot,
jirhiker,
kbighorse and
ksmuczynski
There was a problem hiding this comment.
Pull request overview
This PR updates the ChemistrySampleInfo model to align with the NM_Aquifer table structure by changing the primary key from OBJECTID to SamplePtID.
Changes:
- Changed
sample_pt_idfrom an optional string to a required UUID primary key - Made
object_idnullable instead of being the primary key - Updated the upsert conflict resolution to use
SamplePtIDinstead ofOBJECTID
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| db/nma_legacy.py | Updated ChemistrySampleInfo model to make sample_pt_id the primary key as a UUID and made object_id nullable |
| transfers/backfill/chemistry_sampleinfo.py | Changed upsert conflict index from OBJECTID to SamplePtID and swapped update fields accordingly |
| set_={ | ||
| "SamplePointID": excluded.SamplePointID, | ||
| "SamplePtID": excluded.SamplePtID, | ||
| "OBJECTID": excluded.OBJECTID, |
There was a problem hiding this comment.
Since SamplePtID is now the primary key used in conflict resolution, it should not be included in the set_ dictionary for updates. Primary key fields should not be updated during an upsert operation.
| "OBJECTID": excluded.OBJECTID, |
There was a problem hiding this comment.
SamplePtID is the index_element and OBJECTID is now in the set_.
|
You need to also update the alembic script that creates the chemistry sampleinfo table |
make `sample_pt_id` the primary key and change its type to UUID
jacob-a-brown
requested review from
Copilot and
jirhiker
and removed request for
jirhiker
| "SamplePtID", UUID(as_uuid=True), nullable=False, primary_key=True | ||
| ) | ||
|
|
||
| object_id: Mapped[int] = mapped_column("OBJECTID", nullable=True) |
There was a problem hiding this comment.
Since OBJECTID is no longer the primary key, it should have an explicit Integer type specification for clarity and consistency with the previous definition.
| object_id: Mapped[int] = mapped_column("OBJECTID", nullable=True) | |
| object_id: Mapped[int] = mapped_column("OBJECTID", Integer, nullable=True) |
There was a problem hiding this comment.
All other models define the type in Mapped[<type>], not in the invocation of mapped_column. Other legacy models need to be updated to reflect the overall repo style. That being said, there are some instances where the type specification needs to go into mapped_column, such as when defining string lengths (e.g. String(50)) and for UUID invocation.
This goes for nullable, too. That should be defined in mapped_column and not in Mapped.
|
Updates to |
Why
This PR addresses the following problem / context:
SamplePtID, notOBJECTID, is the primary key in NM_AquiferHow
Implementation summary - the following was changed / added / removed:
sample_pt_idfromobject_idin theChemistrySampleInfomodelsample_pt_idto be a UUID to reflect NM_AquiferSamplePtIDfor the insert statement.Notes
Any special considerations, workarounds, or follow-up work to note?
idfields to all legacy tables, but this PR does not address that.